Skip to content

Migrate YearPickerModal to a @react-navigation modal screen#91106

Closed
trasnake87 wants to merge 35 commits into
Expensify:mainfrom
trasnake87:yearpicker-rnav-90469
Closed

Migrate YearPickerModal to a @react-navigation modal screen#91106
trasnake87 wants to merge 35 commits into
Expensify:mainfrom
trasnake87:yearpicker-rnav-90469

Conversation

@trasnake87

@trasnake87 trasnake87 commented May 19, 2026

Copy link
Copy Markdown
Contributor

Explanation of Change

YearPickerModal was a react-native-modal (RIGHT_DOCKED) rendered inside CalendarPicker, driven by local isYearPickerVisible/currentDateView state. Because react-native-modal mounts and animates independently of the native stack, it animated inconsistently with the surrounding @react-navigation screens (the problem described in the parent migration #53493).

This migrates the year picker to a dynamic @react-navigation modal route, following the pattern established by PR #88915 (DynamicExpenseLimitTypeSelectorPage):

  • Adds DYNAMIC_ROUTES.YEAR_SELECTOR and a standalone DynamicYearSelectorPage screen (the year list + search input, built from route query params), wired through ROUTES, SCREENS, ModalStackNavigators, linkingConfig, and navigation types.
  • Lifts the picker result out of CalendarPicker's local state into a transient Onyx key (CALENDAR_PICKER_SELECTED_YEAR), tagged with a per-instance contextID. CalendarPicker is rendered in multiple places, and RangeDatePicker mounts two instances simultaneously, so the result is matched back to the instance that opened the picker via contextID and cleared once consumed.
  • CalendarPicker now navigates to the year route (via createDynamicRoute) and applies the returned year to the displayed date. Hosts that render CalendarPicker inside a popover (DatePickerModal, the Search date dropdown) pass onBeforeOpenYearPicker so the popover is dismissed before the year screen is shown, instead of stacking a screen behind a react-native-modal.
  • Removes YearPickerModal and the now-unused year-list state in CalendarPicker.

Fixed Issues

$ #90469
PROPOSAL: #90469 (comment)

Tests

  1. Open Settings → Profile → Date of birth (or any date field, e.g. an expense's date).
  2. Tap the date field to open the calendar.
  3. Tap the year in the calendar header.
  4. Verify the year selector opens as a screen that slides in like other native screens (not a modal that animates independently), with a working search box.
  5. Select a year and verify you return to the date flow and the calendar is showing the year you picked.
  6. Repeat for a Search date filter (Advanced filters → a date filter → switch to a single date / range) and for Schedule a call, and verify the year selector works in each and updates the correct calendar (for a range, the calendar you opened it from).
  7. Verify the month picker still opens and works as before.
  • Verify that no errors appear in the JS console

Offline tests

The year selector is client-side navigation and writes only to a transient local Onyx value; behavior is identical offline.

  • Verify that no errors appear in the JS console

QA Steps

Same as tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
and_91106_trim.mp4
Android: mWeb Chrome
web_91106_mweb_20260521_040950.mp4
iOS: Native
ios_91106.mp4
iOS: mWeb Safari
ios_mweb_91106_20260529_080043_cropped.mp4
MacOS: Chrome / Safari
web_91106_20260519_202020.mp4

@trasnake87
trasnake87 requested review from a team as code owners May 19, 2026 17:39
@melvin-bot

melvin-bot Bot commented May 19, 2026

Copy link
Copy Markdown

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@melvin-bot
melvin-bot Bot requested review from flaviadefaria and parasharrajat and removed request for a team May 19, 2026 17:40
@melvin-bot

melvin-bot Bot commented May 19, 2026

Copy link
Copy Markdown

@parasharrajat Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9781e56e2a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/ROUTES.ts Outdated
Comment on lines +110 to +122
entryScreens: [
SCREENS.SETTINGS.PROFILE.DATE_OF_BIRTH,
SCREENS.SETTINGS.PROFILE.STATUS_CLEAR_AFTER_DATE,
SCREENS.MONEY_REQUEST.STEP_DATE,
SCREENS.MONEY_REQUEST.STEP_TIME,
SCREENS.MONEY_REQUEST.STEP_TIME_EDIT,
SCREENS.MONEY_REQUEST.SPLIT_EXPENSE_CREATE_DATE_RANGE,
SCREENS.SEARCH.ROOT,
SCREENS.SEARCH.ADVANCED_FILTERS_DATE_RHP,
SCREENS.SEARCH.EDIT_MULTIPLE_DATE_RHP,
SCREENS.CHRONOS_SCHEDULE_OOO_ROOT,
SCREENS.SCHEDULE_CALL,
],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add missing YEAR_SELECTOR entry screens for DatePicker flows

CalendarPicker now always opens the year chooser through a dynamic route, and getStateFromPath() only authorizes that route when the current screen is in DYNAMIC_ROUTES.YEAR_SELECTOR.entryScreens; otherwise it falls back as an unauthorized dynamic route. The new allowlist here omits several screens that still render DatePicker (SCREENS.SETTINGS.WALLET.PERSONAL_CARD_EDIT_TRANSACTION_START_DATE, SCREENS.WORKSPACE.COMPANY_CARDS_ASSIGN_CARD_TRANSACTION_START_DATE, SCREENS.WORKSPACE.COMPANY_CARD_EDIT_TRANSACTION_START_DATE, and SCREENS.DEBUG.DYNAMIC_DETAILS_DATE_TIME_PICKER_PAGE), so tapping the calendar year on those pages will not navigate to the new year selector.

Useful? React with 👍 / 👎.

@trasnake87

Copy link
Copy Markdown
Contributor Author

Good catch on the entry screens. Rather than extend the allowlist, I switched YEAR_SELECTOR to entryScreens: ['*'].

Reasoning: CalendarPicker is a generic component reachable from a lot of screens — direct date input fields plus DateSelectPopup, RangeDatePicker, DatePresetFilterBase, ScheduleCallPage, the company/personal card transaction start date pages, the debug datetime picker, and so on. The previous in-place YearPickerModal had no screen restriction, so an explicit allowlist is both a behavior change and inherently fragile — it would silently break the year picker again whenever a new date-input screen is added. The '*' wildcard keeps it reachable from every host and matches the existing generic flows (KEYBOARD_SHORTCUTS, EXIT_SURVEY_*).

Added a regression test in CalendarPickerTest.tsx asserting the route stays unrestricted so this can't quietly regress to a partial allowlist. jest and eslint are green (83b38b6).

@trasnake87
trasnake87 force-pushed the yearpicker-rnav-90469 branch from 83b38b6 to 2a4c5ce Compare May 20, 2026 03:34
Replace the react-native-modal based YearPickerModal, rendered inside
CalendarPicker, with a dynamic @react-navigation route so it animates
consistently with the surrounding native stack.

- Add the DYNAMIC_YEAR_SELECTOR dynamic route and the standalone
  DynamicYearSelectorPage screen (year list + search), built from
  route query params.
- Lift the picker result out of CalendarPicker local state into a
  transient Onyx key, tagged with a per-instance contextID so the
  correct CalendarPicker consumes it (range pickers mount two).
- CalendarPicker navigates to the year route and applies the returned
  year; hosts that render it inside a popover pass onBeforeOpenYearPicker
  to dismiss the popover before the year screen is shown.
- Remove YearPickerModal and the now-unused year list state.
- Update CalendarPicker tests for the navigation flow and add coverage
  for the year selection action.
@trasnake87
trasnake87 force-pushed the yearpicker-rnav-90469 branch from 2a4c5ce to 364652d Compare May 20, 2026 19:11
CalendarPicker is a generic component reached from many screens (date input
fields, DateSelectPopup, RangeDatePicker, DatePresetFilterBase,
ScheduleCallPage, ...). The previous in-place YearPickerModal had no screen
restriction, so a hand-maintained entryScreens allowlist silently broke the
year picker on omitted screens (e.g. company/personal card transaction start
date pages, debug datetime picker). Use the '*' wildcard so the year selector
stays reachable from every CalendarPicker host and does not regress when new
date-input screens are added. Added a regression test asserting the route
remains unrestricted.
@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines Coverage Δ
src/ONYXKEYS.ts 100.00% <ø> (ø)
src/ROUTES.ts 21.53% <100.00%> (+0.05%) ⬆️
src/SCREENS.ts 100.00% <ø> (ø)
src/components/DatePicker/CalendarPicker/index.tsx 99.25% <100.00%> (+2.67%) ⬆️
src/components/DatePicker/DatePickerModal.tsx 100.00% <100.00%> (ø)
src/components/DatePicker/useIsYearSelectorOpen.ts 100.00% <100.00%> (ø)
...ents/Modal/ReanimatedModal/OverlayHiddenContext.ts 100.00% <100.00%> (ø)
...s/Search/FilterComponents/DatePresetFilterBase.tsx 61.26% <100.00%> (+0.27%) ⬆️
...onents/Search/FilterComponents/RangeDatePicker.tsx 95.45% <100.00%> (+95.45%) ⬆️
src/libs/ExportOnyxState/common.ts 80.35% <ø> (ø)
... and 13 more
... and 228 files with indirect coverage changes

@trasnake87
trasnake87 force-pushed the yearpicker-rnav-90469 branch from 364652d to 883930e Compare May 20, 2026 19:22
@parasharrajat

Copy link
Copy Markdown
Member

Fix the checklist @trasnake87 and add vids for all platforms.

@trasnake87

Copy link
Copy Markdown
Contributor Author

Added the Android: mWeb Chrome video and ticked the offline-steps box (the section was already in the body, just hadn't been checked). iOS to follow.

const [currentDateView, setCurrentDateView] = useState(() => getInitialCurrentDateView(value, minDate, maxDate));
const [isYearPickerVisible, setIsYearPickerVisible] = useState(false);
const [isMonthPickerVisible, setIsMonthPickerVisible] = useState(false);
const [fallbackContextID] = useState(() => `calendarPicker-${Math.random().toString(36).slice(2)}`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useREf

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on the unused useState setter. I switched the fallback identifier to useId() rather than useRef: contextID is read during render, and the repo's React Compiler lint rules reject reading ref.current during render. useId() gives a stable per-instance id with no unused setter, so it's the cleaner fit here. (426a100)

}
const {year} = selectedYearResult;
clearCalendarPickerSelectedYear();
requestAnimationFrame(() => setCurrentDateView((prev) => setYear(new Date(prev), year)));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need requestAnimationFrame here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect reacts to the year that the year picker screen writes into Onyx. Calling setCurrentDateView synchronously in the effect body trips react-hooks/set-state-in-effect (the cascading-render rule), so it's deferred one frame. I added a comment in 426a100 making that explicit.

onSelected={handleSingleDateSelected}
minDate={CONST.CALENDAR_PICKER.MIN_DATE}
maxDate={CONST.CALENDAR_PICKER.MAX_DATE}
pickerContextID="searchSingleDate"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just leave these to CalendarPicker auto-calculated values. What is the benefit of passing static values?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallbackContextID is regenerated each time a CalendarPicker mounts. This host's popover is dismissed when the year picker opens, so the CalendarPicker unmounts and remounts fresh when it's reopened — a regenerated fallback wouldn't match the contextID the year picker tagged its result with in Onyx, and the selection would be dropped. A stable static id keeps that round-trip intact (and the range picker needs two distinct ids so its calendars don't pick up each other's result).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should remove the fallback ID as it is prone to this bug. we should make sure the contextID is provided from parent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cd94220pickerContextID is now required on CalendarPicker, the useId() fallback and the pickerContextID ?? fallbackContextID coalescence are gone, and useEffect / openYearPicker reference pickerContextID directly. All five product call sites (ScheduleCallPage, DatePresetFilterBase, RangeDatePicker × 2, DatePickerModal) were already passing stable ids, so this just tightens the contract. Added a small CalendarPickerForTest wrapper in tests/unit/CalendarPickerTest.tsx so existing tests that don't exercise the contextID still satisfy the required prop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cd94220pickerContextID is now a required prop on CalendarPicker (no internal useId fallback), so every host has to provide a stable ID explicitly. The DatePresetFilterBase call site still passes pickerContextID="searchSingleDate" since that is now the explicit parent-provided ID.

forceVerticalCalendars?: boolean;

/** Called right before navigating to the year picker screen (used by popover hosts to dismiss themselves) */
onBeforeOpenYearPicker?: () => void;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of this, you can call Modal.closeTop from modal action file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 426a100CalendarPicker now calls Modal.closeTop() directly instead of taking a close callback from each host. I gated it behind a boolean prop (shouldCloseModalOnYearPickerOpen): the wide-screen popover hosts set it, but the full-screen mobile DateSelectPopup overlay deliberately stays mounted while the year picker is open, so the dismissal needs to stay opt-in rather than fire unconditionally from CalendarPicker.

@trasnake87

Copy link
Copy Markdown
Contributor Author

@parasharrajat I've added the iOS native recording to the Screenshots/Videos section. The flow shows opening Search → Date filter → Custom date and tapping the year label, which now navigates to the migrated @react-navigation Year screen (full-screen page with its own header + Back, replacing the old react-native-modal). Selecting 2024 returns to the calendar with the new year applied.

Web/Android boxes were already ticked with their respective recordings; iOS row is now ticked too.

Comment thread src/ROUTES.ts
// previous in-place YearPickerModal had no screen restriction. Use '*' so the year selector
// remains reachable from every CalendarPicker host and doesn't silently break when new
// date-input screens are added (matches KEYBOARD_SHORTCUTS / EXIT_SURVEY_* generic flows).
entryScreens: ['*'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a getRoute handler here to add queryParams.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cd94220 — added a getRoute on YEAR_SELECTOR that takes {contextID, currentYear, minYear, maxYear} and goes through getUrlWithParams('year-selector', …) so encoding stays in one place and matches the declared queryParams.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cd94220YEAR_SELECTOR now has a typed getRoute({contextID, currentYear, minYear, maxYear}) handler that builds the query params via getUrlWithParams.

closeTop();
}
Navigation.navigate(
createDynamicRoute(`${DYNAMIC_ROUTES.YEAR_SELECTOR.path}?contextID=${encodeURIComponent(contextID)}&currentYear=${currentYearView}&minYear=${minYear}&maxYear=${maxYear}`),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the getRoute handler here to create the route path

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cd94220openYearPicker now calls DYNAMIC_ROUTES.YEAR_SELECTOR.getRoute({contextID: pickerContextID, currentYear: currentYearView, minYear, maxYear}) and passes that into createDynamicRoute. Existing CalendarPickerTest assertions (year-selector, contextID=…, currentYear=…) still pass.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in cd94220 — this site now uses DYNAMIC_ROUTES.YEAR_SELECTOR.getRoute({contextID: pickerContextID, currentYear: currentYearView, minYear, maxYear}) instead of the manual string concatenation.

Make pickerContextID required so every CalendarPicker host owns a stable id;
remove the useId-based fallback that regenerated on mount and could mis-route
the year picker's selection back to a remounted instance.

Add a getRoute handler on the YEAR_SELECTOR dynamic route and use it from
CalendarPicker.openYearPicker so the path/queryParams stay co-located with
the route definition.
Comment on lines +122 to +135
useEffect(() => {
isYearSelectorOpenRef.current = isYearSelectorOpen;
if (isYearSelectorOpen) {
yearSelectorGraceRef.current = true;
return;
}
if (!yearSelectorGraceRef.current) {
return;
}
const graceTimeoutID = setTimeout(() => {
yearSelectorGraceRef.current = false;
}, 250);
return () => clearTimeout(graceTimeoutID);
}, [isYearSelectorOpen]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? can you add a comment here on the effect?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added. This mirrors the selector state into a ref plus a short grace window so closeDatePicker can ignore the trailing popstate: selecting a year does a goBack, and its popstate reaches the close listener a beat after isYearSelectorOpen has flipped back to false — gating on the live flag alone would close the picker before the picked year is applied.

return (
<View
pointerEvents="box-none"
pointerEvents={isHiddenForOverlay ? 'none' : 'box-none'}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isOverlayHidden would be a better name. Update the naming similiarly everywhere such as Context as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed — isOverlayHidden for the state and OverlayHiddenContext for the context, updated everywhere (imports, files, tests).

Comment on lines +265 to +267
ref={(node) => {
modalContentRef.current = node;
}}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ref={(node) => {
modalContentRef.current = node;
}}
ref={modalContentRef}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — passing modalContentRef directly now. Typed the ref as Modal for the prop and read it back as unknown in the effect, since on web RNW actually stashes an HTMLElement there.

Comment on lines +263 to +264
// On web, RNW forwards this ref to ModalContent's outermost element (an HTMLElement); on native
// it resolves to the Modal instance and the instanceof guard in the effect above makes it a no-op.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// On web, RNW forwards this ref to ModalContent's outermost element (an HTMLElement); on native
// it resolves to the Modal instance and the instanceof guard in the effect above makes it a no-op.

Remove it here. Too much comment pollution

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed — kept a one-line pointer instead.

swipeDirection={swipeDirection}
>
{children}
<HiddenForOverlayContext.Provider value={setIsHiddenForOverlay}>{children}</HiddenForOverlayContext.Provider>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct to render provider here. the year picker will open in its own page under the stack and will this provider wrap that if rendered here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional: the provider wraps only the modal's own children (the popover content, including CalendarPicker). The year selector opens as its own @react-navigation screen in the stack — not a descendant of the modal — so it isn't wrapped, and doesn't need to be: the only consumer is the in-modal CalendarPicker asking its host to hide while the selector is up.

Comment on lines +60 to +85

// Opening the year picker blurs the Search screen and unmounts this popover, resetting selectedDateModifier
// (the Custom date/range sub-view) back to the top menu on return. Persist it and restore it on return (a
// pending year write-back for a search calendar) so the calendar reopens with the picked year applied.
const [storedDateModifier] = useOnyx(ONYXKEYS.CALENDAR_PICKER_SELECTED_DATE_MODIFIER);
const [storedYearSelection] = useOnyx(ONYXKEYS.CALENDAR_PICKER_SELECTED_YEAR);
const hasRestoredDateModifierRef = useRef(false);

useEffect(() => {
if (!selectedDateModifier) {
return;
}
setCalendarPickerSelectedDateModifier(selectedDateModifier);
}, [selectedDateModifier]);

useEffect(() => {
if (hasRestoredDateModifierRef.current || selectedDateModifier || !storedDateModifier || !storedYearSelection?.contextID.startsWith('search')) {
return;
}
const dateModifierToRestore = Object.values(CONST.SEARCH.DATE_MODIFIERS).find((modifier) => modifier === storedDateModifier);
if (!dateModifierToRestore) {
return;
}
hasRestoredDateModifierRef.current = true;
requestAnimationFrame(() => setSelectedDateModifier(dateModifierToRestore));
}, [storedDateModifier, storedYearSelection, selectedDateModifier]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good if we moved all the logic inside the main yearpicker hook we discussed in the previous comment.

Currently, it seems that we are adding many different pieces to different components to open the year picker and manage temporary state.

It would be good to have it work like this:

  1. We create a hook with the logic related to the year picker opening as well as the internal state management required.
  2. We use this hook in the component that needs to open the year picker, and the hook controls all the side effects and internal state.
  3. it exposes necessary functions to manage internal state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — that bookkeeping now lives in a small useSearchYearSelector hook layered on the same year-selector Onyx keys (persists/restores the open sub-view, exposes clearBreadcrumb), so DateSelectPopup no longer carries it. Kept it separate from the generic useYearSelector since the date-modifier state only exists for the search filter.

@parasharrajat

Copy link
Copy Markdown
Member

Have you tested this logic after refreshing the page? Does it work after we refresh the year picker page?

@trasnake87

Copy link
Copy Markdown
Contributor Author

@parasharrajat Both done ✅

Checks fixed — the PR had drifted ~2 weeks behind main, which switched formatters (Oxfmt), tightened several lint rules, and moved the dev toolchain to rsbuild in the meantime. Merged latest main (head 723ffd0), reformatted the pre-migration files, aligned the one affected test with the new lint rules, and re-verified: 31/31 checks passing (the one pending/skip is the reviewer-checklist + an unrelated skip), tsc clean, all 6 year-selector suites green (89 tests).

Refresh — tested live on this branch, works end-to-end:

  1. Opened the DOB date picker → tapped the year → the year-selector RHP opened (…/year-selector?contextID=datePicker-dob&currentYear=2008…).
  2. Hit browser refresh while on the year-selector route → the page re-renders standalone straight from the URL (all params — context, current/min/max year — live in the route), with the current year pre-selected. No crash, no dead end.
  3. Selected a different year (2005) → navigated back to Personal details. The popover itself doesn't survive a hard reload (no popover does — same as main today), but the selection is written to Onyx keyed by contextID before navigation, so:
  4. Reopening the date picker → the calendar opens on July 2005 — the selection made across the reload is consumed and applied on remount.

So the flow is refresh-proof by construction: the selector is a real route (URL carries its state), and the hand-back is a persisted, context-routed write rather than in-memory popover state. Happy to record a video of the sequence if useful.

@parasharrajat

Copy link
Copy Markdown
Member

Check my other comments.

@trasnake87

Copy link
Copy Markdown
Contributor Author

@parasharrajat Friendly check-in — everything from Saturday's list is in place: checks green (31/31), and the refresh behavior verified live (selection survives a reload via the contextID write-back). Happy to jump on anything else the same day you flag it.

@parasharrajat

parasharrajat commented Jul 16, 2026

Copy link
Copy Markdown
Member

@trasnake87 can you review these #91106 (review) ?

# Conflicts:
#	src/components/Search/FilterDropdowns/FilterPopupButton.tsx
#	src/components/Search/FilterDropdowns/SearchAdvancedFiltersPopup/index.tsx
…context

- Extract the CalendarPicker year-selector side effects into a dedicated
  useYearSelector hook (write-back subscription, host-modal coordination, and
  the self-hide computation) exposing an openYearPicker method, so the calendar
  component only renders the calendar.
- Fold the Search date-filter's year-picker temporary state (persist/restore the
  open sub-view) into a thin useSearchYearSelector hook layered on the same
  Onyx keys, removing that bookkeeping from DateSelectPopup.
- Rename HiddenForOverlayContext to OverlayHiddenContext and the modal state
  isHiddenForOverlay to isOverlayHidden across the component, tests, and file
  names for clearer naming.
- Use the modalContentRef directly on the Modal and trim the ref-forwarding
  comment in ReanimatedModal.
- Document the DatePicker year-selector grace-window effect.
@trasnake87

Copy link
Copy Markdown
Contributor Author

@parasharrajat All seven addressed in 1c68db0 (plus a merge of latest main — the FilterPopupButton lazy-mount gate from main is composed with our keep-mounted requirement as (isFocused || isOverlayVisible) && hasEverExpanded). The effect logic now lives in useYearSelector / useSearchYearSelector hooks per your suggestion, naming is isOverlayHidden/OverlayHiddenContext throughout, and the provider-placement question is answered on the thread. 90 tests green across the seven touched suites, tsc/lint clean. Apologies these sat since the 11th — I missed the review batch; that's fixed on my end.

The inline navigation-state selector arrow was memoized by the OXC compiler
but not by Babel, which the React Compiler compliance check flags as a new
divergence on this added file. Extract the selector to a module-level named
function so both compilers agree (neither memoizes it), preserving behavior.
The extracted useYearSelector and useSearchYearSelector hooks exported their
Params/Result type aliases, but nothing imports them outside their own files.
knip flagged the four exports as new unused-type findings. The types are still
defined and used internally as the hooks' parameter and return annotations;
only the redundant public exports are removed.
CALENDAR_PICKER_SELECTED_YEAR and CALENDAR_PICKER_SELECTED_DATE_MODIFIER were
added to ONYXKEYS but not sorted into any export bucket, so the ExportOnyxState
coverage test (every key must belong to exactly one bucket) failed. Both hold
transient, non-sensitive UI state (a year plus a context id, and a date-filter
sub-view flag), so add them to safeOnyxKeys in alphabetical order.
@parasharrajat

Copy link
Copy Markdown
Member

being discussed on main issue.

@parasharrajat

Copy link
Copy Markdown
Member

We won't need this PR #53493 (comment)

@mountiny

Copy link
Copy Markdown
Contributor

Confirmed

@mountiny mountiny closed this Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants